srv_resolver: add 15s timeout to DNS lookups #19026
Conversation
|
|
amaanq
force-pushed
the
srv-timeout
branch
3 times, most recently
from
5059cd5 to
aaacc87
Compare
| raise e | ||
| except defer.TimeoutError as e: | ||
| raise defer.TimeoutError( | ||
| f"Could not resolve DNS for SRV record {service_name!r} due to timeout (50s total)" |
There was a problem hiding this comment.
| f"Could not resolve DNS for SRV record {service_name!r} due to timeout (50s total)" | |
| f"Timed out while trying to resolve DNS for SRV record for {service_name!r} (timeout=15s)" |
Does this sound better?
Only real nit is this previously said 50s total vs our new 15s timeout. Ideally, we'd have a constant to use here but I'm not sure that moving timeout=(1, 3, 3, 3, 5) to the top as a constant is better. The comments probably read better in place where it's used.
There was a problem hiding this comment.
I'd say to make it a constant, then its easy to do the following:
| f"Could not resolve DNS for SRV record {service_name!r} due to timeout (50s total)" | |
| f"Timed out while trying to resolve DNS for SRV record for {service_name!r} (timeout={sum(LOOKUP_TIMEOUTS)}s)" |
There was a problem hiding this comment.
I like Jonathan's suggestion of using a constant, I've gone ahead and applied that suggestion :)
| return list(cache_entry) | ||
| else: | ||
| raise e | ||
| except defer.TimeoutError as e: |
There was a problem hiding this comment.
It would be nice to have a test that stressed this part of the code. Especially since I'm unsure if defer.TimeoutError is actually the exception type raised here.
Do you think you would be up for that? Probably involves reactor.advance(15 + 1) to advance time past the timeout. Otherwise, I can take a stab at it.
Co-authored-by: Jonathan de Jong <[email protected]>
4 participants
synapse.http.federation.srv_resolver.SrvResolver.resolve_serviceisn't able to "timeout" properly, and thus stalls federation #9774Problem
The default timeout when looking up SRV records for the dns client is 60 seconds, which is quite long and can be problematic as that's the value of the default http client timeout, thus the error surfaced is the http error and not the underlying DNS error.
Solution
I've added a more aggressive timeout of 15 seconds total (with retries after 1, 3, 3, 3, and 5 seconds) for the SRV lookup. I've also re-labeled the `defer.TimeoutError, as that was done in the prior synapse PR, and makes the error more clear to users.
Pull Request Checklist
EventStoretoEventWorkerStore.".code blocks.